-
Notifications
You must be signed in to change notification settings - Fork 3.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
chore: Visual changes for core navigation elements on IDE #37880
Conversation
WalkthroughThe pull request introduces enhancements to the IDE's sidebar and navigation components, focusing on improving visual hierarchy and user experience. The changes include updating sidebar button styling, adding test IDs for better testability, renaming "Data" to "Datasources", and optimizing component rendering. The modifications span multiple files in the client application, addressing design and testing requirements. Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
/build-deploy-preview |
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/12117552478. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (5)
app/client/src/pages/Editor/IDE/EditorPane/components/SegmentedHeader.tsx (1)
14-16
: Consider using design tokens for font weightsWhile the implementation is clean, consider using design tokens for font weights to maintain consistency across the application.
- font-weight: ${(props) => (props.isSelected ? "500" : "normal")}; + font-weight: ${(props) => (props.isSelected ? "var(--ads-v2-font-weight-bold)" : "var(--ads-v2-font-weight-normal)")};app/client/src/pages/Editor/IDE/Sidebar.tsx (1)
29-30
: Enhance comment clarityThe current comment could be more descriptive about the purpose and impact of this configuration.
- // Updates the bottom button config based on datasource existence + // Modifies bottom navigation buttons to show warning state when no datasources existapp/client/src/IDE/Components/Sidebar/SidebarButton/SidebarButton.tsx (2)
Line range hint
7-13
: Consider creating a ticket for TODO itemsThere are multiple TODO comments about adding configurations for additional conditions.
Would you like me to create a GitHub issue to track the implementation of additional condition configurations?
48-51
: Consider adding transition for smoother hover effectThe hover state could benefit from a smooth transition effect.
const IconContainer = styled.div<{ selected: boolean }>` + transition: background-color 0.2s ease; &:hover { background: var(--ads-v2-color-bg-subtle, #f1f5f9); } `;
app/client/src/ce/entities/IDE/constants.ts (1)
Line range hint
69-89
: Inconsistencies found in BottomButtons configuration
- The DATA button uses
title
while LIBRARIES and SETTINGS usetooltip
. This should be consistent across all buttons.- The DATA button incorrectly uses
SidebarTopButtonTitles.DATA
instead of a bottom button title.Apply this diff to fix the inconsistencies:
export const BottomButtons: IDESidebarButton[] = [ { state: EditorState.DATA, icon: "datasource-v3", - title: SidebarTopButtonTitles.DATA, + tooltip: "Data", urlSuffix: "datasource", }, { state: EditorState.LIBRARIES, icon: "packages-v3", tooltip: SidebarBottomButtonTitles.LIBRARIES, urlSuffix: "libraries", }, { state: EditorState.SETTINGS, icon: "settings-v3", tooltip: SidebarBottomButtonTitles.SETTINGS, urlSuffix: "settings", }, ];
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
app/client/src/IDE/Components/Sidebar/SidebarButton/SidebarButton.tsx
(2 hunks)app/client/src/ce/entities/IDE/constants.ts
(1 hunks)app/client/src/pages/Editor/IDE/EditorPane/components/SegmentedHeader.tsx
(2 hunks)app/client/src/pages/Editor/IDE/Sidebar.tsx
(2 hunks)
🔇 Additional comments (5)
app/client/src/pages/Editor/IDE/EditorPane/components/SegmentedHeader.tsx (1)
36-41
: LGTM: Clean implementation of segmented control labels
The implementation of StyledLabel across all tabs is consistent and provides clear visual feedback for the selected state.
Also applies to: 45-50, 54-59
app/client/src/pages/Editor/IDE/Sidebar.tsx (2)
Line range hint 30-41
: LGTM: Well-structured conditional button configuration
The useMemo implementation and conditional button modification are clean and efficient.
67-71
: LGTM: Clean prop updates
The prop updates correctly reflect the new button configuration structure.
app/client/src/IDE/Components/Sidebar/SidebarButton/SidebarButton.tsx (1)
39-39
: LGTM: Proper use of design tokens
The background color implementation correctly uses the design system tokens.
Also applies to: 50-50
app/client/src/ce/entities/IDE/constants.ts (1)
Line range hint 69-89
: Verify type safety and usage
Let's verify the type safety and usage of these buttons in the codebase.
✅ Verification successful
Changes are type-safe and properly implemented
The modifications to BottomButtons
are consistent with the IDESidebarButton
interface which extends SidebarButtonProps
. The interface requires state
, urlSuffix
, and allows for both title
and tooltip
properties. The implementation is used correctly in the Sidebar component and maintains type safety across the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the IDESidebarButton interface and its usage
# Check the interface definition
ast-grep --pattern 'interface IDESidebarButton {
$$$
}'
# Check usage of BottomButtons
rg -A 5 'BottomButtons'
# Check if there are any direct references to title/tooltip properties
rg -A 2 'button\.(title|tooltip)'
Length of output: 2500
Script:
#!/bin/bash
# Let's check the interface definition and related types
ast-grep --pattern 'interface IDESidebarButton'
# Check for EditorState enum definition
ast-grep --pattern 'enum EditorState'
# Check for SidebarTopButtonTitles and SidebarBottomButtonTitles
ast-grep --pattern 'enum SidebarTopButtonTitles'
ast-grep --pattern 'enum SidebarBottomButtonTitles'
# Check for any other components using similar button structure
rg -A 3 'IDESidebarButton\[\]'
Length of output: 1190
Script:
#!/bin/bash
# Let's check the actual interface definition to verify type safety
rg -A 10 'interface IDESidebarButton'
# Check for any type errors or issues in the implementation
rg -A 2 'type.*error|Type.*error' app/client/src/ce/entities/IDE/
Length of output: 940
/build-deploy-preview |
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/12117780291. |
Deploy-Preview-URL: https://ce-37880.dp.appsmith.com |
1 similar comment
Deploy-Preview-URL: https://ce-37880.dp.appsmith.com |
/build-deploy-preview |
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/12118518647. |
Deploy-Preview-URL: https://ce-37880.dp.appsmith.com |
app/client/src/pages/Editor/IDE/EditorPane/components/SegmentedHeader.tsx
Outdated
Show resolved
Hide resolved
app/client/src/pages/Editor/IDE/EditorPane/components/SegmentedHeader.tsx
Outdated
Show resolved
Hide resolved
app/client/src/pages/Editor/IDE/EditorPane/components/SegmentedHeader.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
app/client/src/IDE/Components/Sidebar/Sidebar.tsx (2)
Line range hint
18-23
: Consider enhancing the interface documentation.The
IDESidebarButton
interface could benefit from JSDoc comments explaining the purpose of each property, especiallystate
andurlSuffix
.+/** + * Interface for IDE sidebar button configuration + * @property state - The editor state this button represents + * @property urlSuffix - The URL suffix for navigation + * @property testId - Unique identifier for testing + */ export interface IDESidebarButton extends Omit<SidebarButtonProps, "onClick" | "selected"> { state: EditorState; urlSuffix: string; }
Line range hint
33-69
: Consider implementing error boundaries for the sidebar navigation.Since this is a core navigation component, it would be beneficial to implement error boundaries to gracefully handle any rendering failures.
Would you like me to provide an example implementation of an error boundary component for the sidebar?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/client/src/IDE/Components/Sidebar/Sidebar.tsx
(2 hunks)app/client/src/IDE/Components/Sidebar/SidebarButton/SidebarButton.tsx
(4 hunks)app/client/src/ce/entities/IDE/constants.ts
(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- app/client/src/IDE/Components/Sidebar/SidebarButton/SidebarButton.tsx
- app/client/src/ce/entities/IDE/constants.ts
🔇 Additional comments (1)
app/client/src/IDE/Components/Sidebar/Sidebar.tsx (1)
47-47
: LGTM! Good addition of test IDs for improved testability.
The consistent implementation of testId
props across both top and bottom button sections enhances the component's testability.
Also applies to: 62-62
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/client/src/IDE/Components/Sidebar/SidebarButton/SidebarButton.tsx (1)
48-54
: Consider consolidating hover stylesThe hover styles could be simplified by using CSS custom properties for the background colors.
- &[data-selected="false"] { - background-color: white; - - &:hover { - background-color: var(--ads-v2-color-bg-subtle, #f1f5f9); - } - } + &[data-selected="false"] { + background-color: var(--ads-v2-color-bg-default, white); + + &:hover { + background-color: var(--ads-v2-color-bg-subtle); + } + }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/client/cypress/support/Pages/IDE/Sidebar.ts
(1 hunks)app/client/src/IDE/Components/Sidebar/SidebarButton/SidebarButton.test.tsx
(1 hunks)app/client/src/IDE/Components/Sidebar/SidebarButton/SidebarButton.tsx
(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
app/client/cypress/support/Pages/IDE/Sidebar.ts (1)
Pattern app/client/cypress/**/**.*
: Review the following e2e test code written using the Cypress test library. Ensure that:
- Follow best practices for Cypress code and e2e automation.
- Avoid using cy.wait in code.
- Avoid using cy.pause in code.
- Avoid using agHelper.sleep().
- Use locator variables for locators and do not use plain strings.
- Use data-* attributes for selectors.
- Avoid Xpaths, Attributes and CSS path.
- Avoid selectors like .btn.submit or button[type=submit].
- Perform logins via API with LoginFromAPI.
- Perform logout via API with LogOutviaAPI.
- Perform signup via API with SignupFromAPI.
- Avoid using it.only.
- Avoid using after and aftereach in test cases.
- Use multiple assertions for expect statements.
- Avoid using strings for assertions.
- Do not use duplicate filenames even with different paths.
- Avoid using agHelper.Sleep, this.Sleep in any file in code.
🔇 Additional comments (3)
app/client/cypress/support/Pages/IDE/Sidebar.ts (1)
5-5
: LGTM! Follows Cypress best practices
The change from class-based selector to data-testid attribute follows Cypress best practices for reliable test selectors.
app/client/src/IDE/Components/Sidebar/SidebarButton/SidebarButton.test.tsx (1)
14-14
: LGTM! Good test coverage for the new feature
The test case properly verifies the testId implementation using React Testing Library's best practices.
Also applies to: 18-22
app/client/src/IDE/Components/Sidebar/SidebarButton/SidebarButton.tsx (1)
19-19
: LGTM! Clean implementation of testId prop
The testId prop is properly typed and consistently implemented in the component.
Also applies to: 92-92
app/client/src/IDE/Components/Sidebar/SidebarButton/SidebarButton.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (3)
app/client/packages/design-system/ads/src/Templates/EntityExplorer/EditorSegments/EditorSegments.styles.ts (2)
8-14
: LGTM! Consider adding documentation.The implementation correctly uses CSS variables and data attributes for state management.
Consider adding a brief comment explaining the styling decision:
+ // Bold text in selected segments for better visual hierarchy .ads-v2-segmented-control__segments-container-segment { &[data-selected="true"] { span { font-weight: var(--ads-v2-font-weight-bold); } } }
8-14
: Consider enhancing accessibility with hover and focus states.The selected state is well-handled, but the component could benefit from additional interactive states.
.ads-v2-segmented-control__segments-container-segment { + &:hover { + background-color: var(--ads-v2-color-bg-subtle); + } + &:focus-visible { + outline: 2px solid var(--ads-v2-color-border-emphasis); + outline-offset: -2px; + } &[data-selected="true"] { span { font-weight: var(--ads-v2-font-weight-bold); } } }app/client/src/pages/Editor/IDE/EditorPane/components/SegmentSwitcher.tsx (1)
11-29
: Consider further optimizations for the segmentOptions.While the useMemo implementation is good, consider these improvements:
- Since the options are static, they could be moved outside the component
- Adding TypeScript interfaces would improve type safety
+interface SegmentOption { + label: string; + startIcon: string; + value: EditorEntityTab; +} +const SEGMENT_OPTIONS: SegmentOption[] = [ + { + label: createMessage(EDITOR_PANE_TEXTS.queries_tab), + startIcon: "queries-line", + value: EditorEntityTab.QUERIES, + }, + // ... rest of the options +]; const SegmentSwitcher = () => { // ... other code - const segmentOptions = useMemo(() => { - return [ - // ... options - ]; - }, []); + const segmentOptions = SEGMENT_OPTIONS;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/client/cypress/support/Pages/IDE/Sidebar.ts
(1 hunks)app/client/packages/design-system/ads/src/Templates/EntityExplorer/EditorSegments/EditorSegments.styles.ts
(1 hunks)app/client/src/IDE/Components/Sidebar/SidebarButton/SidebarButton.tsx
(4 hunks)app/client/src/pages/Editor/IDE/EditorPane/components/SegmentSwitcher.tsx
(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- app/client/cypress/support/Pages/IDE/Sidebar.ts
- app/client/src/IDE/Components/Sidebar/SidebarButton/SidebarButton.tsx
🔇 Additional comments (2)
app/client/src/pages/Editor/IDE/EditorPane/components/SegmentSwitcher.tsx (2)
Line range hint 1-10
: LGTM! Clean imports and component setup.
31-37
: LGTM! Clean component implementation.
The EditorSegments component is properly configured with the necessary props.
@@ -4,5 +4,13 @@ import { Flex } from "../../../Flex"; | |||
export const Container = styled(Flex)` | |||
.editor-pane-segment-control { | |||
max-width: 247px; | |||
|
|||
.ads-v2-segmented-control__segments-container-segment { | |||
&[data-selected="true"] { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
doing this only for Entity explorer? Why not Segmented control itself?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/client/packages/design-system/ads/src/SegmentedControl/SegmentedControl.styles.tsx (1)
Line range hint
90-99
: Consider simplifying the complex selector for better maintainability.While the separator implementation works, the selector complexity (
&:not(:hover):not(:last-child):not([data-selected="true"]):not(:has(+ [data-selected="true"]))
) might make future maintenance challenging.Consider extracting the separator logic into a separate styled component or using a simpler approach:
- &:not(:hover):not(:last-child):not([data-selected="true"]):not( - :has(+ [data-selected="true"]) - ):after { + &:after { content: ""; position: absolute; right: 0; width: 1px; height: 16px; background-color: var(--ads-v2-colors-control-field-default-border); + opacity: 0; } + + &:not(:hover):not(:last-child):not([data-selected="true"]):not( + :has(+ [data-selected="true"]) + ):after { + opacity: 1; + }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/client/packages/design-system/ads/src/SegmentedControl/SegmentedControl.styles.tsx
(2 hunks)
🔇 Additional comments (1)
app/client/packages/design-system/ads/src/SegmentedControl/SegmentedControl.styles.tsx (1)
44-49
: LGTM! Clean implementation of selected state styling.
The bold font weight enhancement for selected segments improves visual hierarchy while maintaining consistency with design system variables.
@@ -8,23 +8,30 @@ const SegmentSwitcher = () => { | |||
const { segment } = useCurrentEditorState(); | |||
const { onSegmentChange } = useSegmentNavigation(); | |||
|
|||
const segmentOptions = useMemo(() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this could just be moved to constants.
## Description Updates a few visual elements for better Navigational experience using the Segmented Controls Fixes #37881 ## Automation /ok-to-test tags="@tag.IDE, @tag.Sanity" ### 🔍 Cypress test results <!-- This is an auto-generated comment: Cypress test results --> > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: <https://github.com/appsmithorg/appsmith/actions/runs/12409098657> > Commit: a94e072 > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=12409098657&attempt=3" target="_blank">Cypress dashboard</a>. > Tags: `@tag.IDE, @tag.Sanity` > Spec: > <hr>Thu, 19 Dec 2024 12:51:29 UTC <!-- end of auto-generated comment: Cypress test results --> ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [ ] No <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Release Notes - **New Features** - Added `testId` property to sidebar buttons for enhanced testing capabilities. - Updated button titles from "Data" to "Datasources" for clarity. - Introduced `BottomButtons` configuration for sidebar buttons based on data source availability. - **Bug Fixes** - Improved visual distinction of selected segments in the Segmented Control. - **Style** - Enhanced styling for sidebar buttons and segments, including hover effects and separators. - **Tests** - Added tests to verify rendering of sidebar buttons with the correct test IDs. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Description
Updates a few visual elements for better Navigational experience using the Segmented Controls
Fixes #37881
Automation
/ok-to-test tags="@tag.IDE, @tag.Sanity"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/12409098657
Commit: a94e072
Cypress dashboard.
Tags:
@tag.IDE, @tag.Sanity
Spec:
Thu, 19 Dec 2024 12:51:29 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Release Notes
New Features
testId
property to sidebar buttons for enhanced testing capabilities.BottomButtons
configuration for sidebar buttons based on data source availability.Bug Fixes
Style
Tests